Fixed bogus lookup logic within list-land `.TransformAsync()`,… by JakenVeina · Pull Request #1171 · reactivemarbles/DynamicData

JakenVeina · 2026-09-01T05:11:06Z

… for processing changes with unspecified indexes, which resulted in Remove and RemoveRange changes with unspecified indexes to be ignored completely.

…ocessing changes with unspecified indexes, which resulted in `Remove` and `RemoveRange` changes with unspecified indexes to be ignored completely.

RemoveWithoutIndex and RemoveRangeWithoutIndex were added by #1171 and dropped while rebasing the global usings onto it.

dwcullop

Fix looks right to me. Matching on EqualityComparer<TSource>.Default lines up with how TransformedItemContainer already defines equality.

The synchronous Transform has the same bug, though, so it'd be good to fix both here:

List/Internal/Transformer.cs:146 and :165 match indexless removes with ReferenceEquals(t.Source, change.Current). For value types that's always false because both sides get boxed, so an indexless Remove/RemoveRange of ints is silently dropped, which is exactly #1169.
The RemoveRange branch there (Where(t => item.Range.Any(...)) into RemoveMany) also removes every duplicate of an item, not one per entry in the range.
TransformFixture.RemoveWithoutIndex only uses Person, so nothing catches it. Mirroring your two new int tests against Transform would.

As it stands sync and async also disagree on what "the same item" means (reference identity vs Equals), which seems worth closing while we're here.

Small nit on the lookup itself: the Select((pair, index) => (sourceItem, (int?)index)).FirstOrDefault(...).index trick takes a second read, allocates per remove, and is duplicated. A plain index loop in one helper that both Transformer and TransformAsync call would be simpler and keeps the two operators from drifting again.

JakenVeina · 2026-10-11T02:41:18Z

Fair point about putting equivalent fixes on .Transform().

I'm not too fussed about the optimalness of the index lookup, when that's SUCH a "this really shouldn't be a thing" scenario, but looking at it, I realized there were actually several OTHER inconsistencies (for other change types) in how index lookups are being done, across both operators. So yeah, I just added a helper method and refactored it into both operators, everywhere an index lookup is needed.

There's still inconsistencies in how the operators handle an index lookup that FAILS (some change types will throw, some will just silently ignore the change) but I opted to just preserve the existing logic, rather than expand scope any further. A failed index lookup really is just an extension of changeset corruption, which is not really this PR's problem to deal with.

…d logic for handling index lookup, when indexes are missing from changesets, into a shared allocation-free helper method.

dwcullop

Heads up that #1192 touches Transformer.cs too and adds a shared UnspecifiedIndexEx with the same rules. Happy for this to merge first. I'll rebase #1192 onto it, move both operators to that helper, and keep UnspecifiedIndexException for a missing Replace target, since that's the better choice.

dwcullop · 2026-10-12T22:49:53Z

                        {
-                            var toRemove = transformed.Where(t => ReferenceEquals(t.Source, t)).ToArray();
-                            transformed.RemoveMany(toRemove);
+                            foreach (var removedItem in item.Range)


This outer foreach (var removedItem in item.Range) repeats the whole removal once per item in the range. With duplicates it removes too much: SourceList [1,2,1,2], RemoveIndex(), RemoveRange(0,2) gives an empty list instead of [1,2]. Without duplicates it's still very slow: removing 500 of 1000 took about 1.4s here versus 1ms for sync Transform. I think the outer loop just needs to go.

Yeah, I'm not sure how I ended up here. Almost looks like a merge conflict.

Fixed.

dwcullop · 2026-10-12T22:49:53Z

                        {
-                            var toRemove = transformed.Where(t => item.Range.Any(current => ReferenceEquals(t.Source, current)));
-                            transformed.RemoveMany(toRemove);
+                            var manyToRemove = new List<TransformedItemContainer>(capacity: item.Range.Count);


Since FindItemIndex already gives us the index, can we just RemoveAt(index) per item rather than collecting and calling RemoveMany? RemoveMany does the lookup again with Join, OrderBy and Duplicates and copies the ChangeAwareList while enumerating it. Same change in TransformAsync.

The idea was supposed to be that we don't break apart the range change into a series of individual changes, but yeah, that's not what's happening anyway. The .RemoveMany() call is binding to an IList<T> extension method, not any method on ChangeAwareList<T> that would be able to optimize.

Fixed.

dwcullop · 2026-10-12T22:49:53Z

+                        var previousIndex = change.PreviousIndex;
+
+                        if (previousIndex < 0)
+                            previousIndex = transformed.FindItemIndex(change.Previous.Value);


If the previous item isn't found, this falls through to transformed[-1] and throws ArgumentOutOfRangeException. Sync throws UnspecifiedIndexException here. Can we throw the same thing so the two match?

Per my initial comment, I left in all the existing discrepancies in failed lookup behavior intact. They're not part of #1169, and and how an operator responds to being given corrupt changesets is really undefined behavior. I'm down with cleaning it up, but not here.

dwcullop · 2026-10-12T22:49:53Z

+        results.HasCompleted.Should().BeTrue("the source, and all asynchronous operations, have completed");
+    }
+
+    // Mirrors coverage for https://github.com/reactivemarbles/DynamicData/issues/1169


Could we add a duplicate-values case (e.g. [1,2,1,2] with an unindexed RemoveRange of [1,2]) and an unindexed Replace for both operators? The duplicates test would have caught the extra loop in the async version. Also, small thing: the // Covers / // Mirrors coverage comments and the trailing whitespace are still there.

I just added some duplicate items to the existing tests. It's not like we lose any coverage by dropping the no-duplicates scenario.

Also, small thing: the // Covers / // Mirrors coverage comments and the trailing whitespace are still there.

Uhhh, yeah, why wouldn't they be? And what trailing whitespace?

dwcullop

Thanks, the RemoveRange fix and the duplicate cases cover what I was worried about. Agreed the lookup-failure discrepancies are out of scope here; I'll line them up in #1192 when I move both operators onto the shared helper.

github-actions · 2026-10-27T03:55:31Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

JakenVeina linked an issue Sep 1, 2026 that may be closed by this pull request

[Bug]: TransformAsync on IChangeSet drops events #1169

Closed

Fixed bogus lookup logic within list-land .TransformAsync(), for pr…

18b0f53

…ocessing changes with unspecified indexes, which resulted in `Remove` and `RemoveRange` changes with unspecified indexes to be ignored completely.

JakenVeina force-pushed the issues/1169 branch from ed51f2a to 18b0f53 Compare September 4, 2026 03:14

JakenVeina requested a review from dwcullop September 24, 2026 03:27

JakenVeina added 7 commits September 28, 2026 00:09

Merge branch 'main' into issues/1169

f13952f

Merge branch 'main' into issues/1169

403d0ec

Merge branch 'main' into issues/1169

06d2dbe

Merge branch 'main' into issues/1169

35551ba

Merge branch 'main' into issues/1169

1dc7af0

Merge branch 'main' into issues/1169

c341cfa

Merge branch 'main' into issues/1169

101fb75

JakenVeina mentioned this pull request Oct 7, 2026

Refactored for global import definitions #1197

Merged

dwcullop mentioned this pull request Oct 9, 2026

Fix list operators that fail on unindexed changes, such as those from RemoveKey #1192

Open

Merge branch 'main' into issues/1169

deddae5

dwcullop added a commit that referenced this pull request Oct 10, 2026

Restore the TransformAsync regression tests for indexless removes

f33c034

RemoveWithoutIndex and RemoveRangeWithoutIndex were added by #1171 and dropped while rebasing the global usings onto it.

dwcullop reviewed Oct 10, 2026

View reviewed changes

JakenVeina force-pushed the issues/1169 branch from 79fb9b0 to 43f46bc Compare October 11, 2026 02:34

Added equivalent fixes (and tests) to .Transform(), and consolidate…

4d81cb1

…d logic for handling index lookup, when indexes are missing from changesets, into a shared allocation-free helper method.

JakenVeina force-pushed the issues/1169 branch from 43f46bc to 4d81cb1 Compare October 11, 2026 02:42

Merge branch 'main' into issues/1169

cd3a996

dwcullop reviewed Oct 12, 2026

View reviewed changes

Review fixes.

387fc12

dwcullop approved these changes Oct 13, 2026

View reviewed changes

JakenVeina merged commit 97ec273 into main Oct 13, 2026
2 checks passed

JakenVeina deleted the issues/1169 branch October 13, 2026 03:00

ViktorJannicke mentioned this pull request Oct 19, 2026

Sync upstream main to 65016e5b (2026-10) Runic-Artifex/DynamicData#10

Merged

github-actions Bot locked as resolved and limited conversation to collaborators Oct 27, 2026

Conversation

JakenVeina commented Sep 1, 2026

Uh oh!

dwcullop left a comment

Choose a reason for hiding this comment

Uh oh!

JakenVeina commented Oct 11, 2026

Uh oh!

dwcullop left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwcullop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Oct 27, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants